[py] Use generated Bidi files instead of hand curated ones#17266
Open
AutomatedTester wants to merge 33 commits intotrunkfrom
Open
[py] Use generated Bidi files instead of hand curated ones#17266AutomatedTester wants to merge 33 commits intotrunkfrom
AutomatedTester wants to merge 33 commits intotrunkfrom
Conversation
selenium-ci
added
C-py
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates Selenium’s Python WebDriver BiDi implementation from hand-curated source files to Bazel-generated modules produced from the BiDi CDDL specification (per #16914), and adjusts supporting build/test infrastructure accordingly.
Changes:
- Remove the hand-maintained
py/selenium/webdriver/common/bidi/*modules and wire Bazel to generate them fromcommon/bidi/spec/*.cddl. - Update Python runtime plumbing to support generated BiDi dataclasses over WebSocket (custom JSON encoding +
RemoteWebDriver.execute()accepting BiDi command generators). - Add/adjust supporting tooling and tests (local dev copy task, CLI alias, minor test robustness tweaks).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/update_copyright.py | Stop excluding a removed BiDi file from copyright updates. |
| rake_tasks/python.rake | Copy generated BiDi files into the source tree for local dev workflows. |
| py/test/selenium/webdriver/common/bidi_browsing_context_tests.py | Make viewport assertions tolerant to minor WM differences. |
| py/test/selenium/webdriver/common/bidi_browser_tests.py | Update imports/constants + locator usage to match generated BiDi APIs. |
| py/selenium/webdriver/remote/websocket_connection.py | Add BiDi-oriented JSON encoding for dataclass payloads when sending WS commands. |
| py/selenium/webdriver/remote/webdriver.py | Allow execute() to accept BiDi command generators and route them via WebSocket. |
| py/selenium/webdriver/common/proxy.py | Add __future__ annotations + formatting adjustments. |
| py/selenium/webdriver/common/by.py | Add __future__ annotations + reformat ByType literal. |
| py/selenium/webdriver/common/bidi/webextension.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/storage.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/session.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/script.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/permissions.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/network.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/log.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/input.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/emulation.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/console.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/common.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/browsing_context.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/browser.py | Remove hand-curated BiDi module (replaced by generated). |
| py/selenium/webdriver/common/bidi/init.py | Remove hand-curated package init (replaced by generated). |
| py/selenium/common/exceptions.py | Formatting-only changes to exception constructors. |
| py/private/generate_bidi.bzl | Add Bazel rule to generate BiDi Python modules from CDDL. |
| py/private/cdp.py | Tighten devtools version directory detection. |
| py/private/_event_manager.py | Add shared event manager used by generated BiDi modules. |
| py/private/BUILD.bazel | Export generator support files for Bazel consumption. |
| py/conftest.py | Add --browser CLI alias for --driver. |
| py/BUILD.bazel | Wire BiDi generation into the Python build graph. |
| py/AGENTS.md | Update Python 3.10+ guidance for agents/contributors. |
| dotnet/src/webdriver/BiDi/EventDispatcher.cs | Refactor event dispatcher internals (currently contains compile-breaking issues). |
| common/bidi/spec/remote.cddl | Add BiDi CDDL “remote” spec inputs for generation. |
| common/bidi/spec/local.cddl | Add BiDi CDDL “local” spec inputs for generation. |
| common/bidi/spec/BUILD.bazel | Export CDDL specs for the Python generator rule. |
Comments suppressed due to low confidence (2)
dotnet/src/webdriver/BiDi/EventDispatcher.cs:43
- The channel is typed as
Channel<EventItem>, but this file only definesPendingEventandEnqueueEventwritesPendingEventinstances. This won’t compile unlessEventItemexists and matches the written type; either switch the channel back toChannel<PendingEvent>or introduce a consistentEventItemtype and use it everywhere.
private readonly Channel<EventItem> _pendingEvents = Channel.CreateUnbounded<EventItem>(new()
{
SingleReader = true,
SingleWriter = true
});
dotnet/src/webdriver/BiDi/EventDispatcher.cs:99
- Inside
ProcessEventsAsync, the loop reads intoevt, but the code still referencesresult.Method/result.EventArgs. This won’t compile and will also read the wrong values. Use the variable you actually read from the channel consistently (and ensure its type matches the channel’s item type).
while (await reader.WaitToReadAsync().ConfigureAwait(false))
{
while (reader.TryRead(out var evt))
{
if (_eventRegistrations.TryGetValue(result.Method, out var registration))
{
foreach (var handler in registration.GetHandlers()) // copy-on-write array, safe to iterate
{
var runningHandlerTask = InvokeHandlerAsync(handler, result.EventArgs);
if (!runningHandlerTask.IsCompleted)
{
Comment on lines
+58
to
+68
| @@ -59,7 +59,13 @@ public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Eve | |||
|
|
|||
| registration.AddHandler(eventHandler); | |||
|
|
|||
| return new Subscription(subscribeResult.Subscription, this, eventHandler); | |||
| return new Subscription(subscribeResult.Subscription, this, eventHandler); | |||
| } | |||
| catch | |||
| { | |||
| registration.RemoveHandler(eventHandler); | |||
| throw; | |||
| } | |||
There was a problem hiding this comment.
SubscribeAsync has a catch block without a corresponding try and the return statement is mis-indented, which will not compile. Wrap the subscription + registration logic in a try and keep the catch immediately after it, or remove the catch if it’s not needed.
cgoldberg
requested changes
Mar 27, 2026
|
|
||
| copy_all = arguments[:all] == 'all' | ||
| if copy_all | ||
| FileUtils.rm_rf("#{lib_path}/common/devtools") |
Member
There was a problem hiding this comment.
I don't think this line is necessary. the next line removes everything under py/selenium/webdriver and copies it
Member
|
We need to add the directory to ruff config in You can add it to: |
Member
|
@AutomatedTester I pushed a fix to your branch that resolves all the comments I made and also adds the generated files to |
cgoldberg
approved these changes
Mar 29, 2026
Contributor
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Member
|
there are still some linting/formatting errors in the new generation code |
AutomatedTester
force-pushed
the
cddl2py_bazel
branch
from
d6d11a4 to
b49a8eb
Compare
cgoldberg
requested changes
Mar 31, 2026
Member
cgoldberg
left a comment
cgoldberg
left a comment
There was a problem hiding this comment.
I fixed python.rake previously, but you force pushed over my commit :/ I just added comments again.
We should add the generated files to .gitignore
Also, run the formatter so it passes the lint job in CI.
| @@ -0,0 +1,1824 @@ | |||
| #!/usr/bin/env python3.10 | |||
| FileUtils.cp(resolved_src, File.join(bidi_dest, entry)) | ||
| end | ||
| end | ||
|
|
||
| %w[common/devtools common/linux common/mac common/windows].each do |dir| |
|
|
||
| resolved_src = File.symlink?(src) ? File.realpath(src) : src | ||
| FileUtils.cp(resolved_src, File.join(bidi_dest, entry)) | ||
| end |
Member
There was a problem hiding this comment.
do FileUtils.rm_f() on each destination file before copying, or this will fail when you run it a 2nd time.
| data = browser_data, | ||
| tags = tags + browser_tags, | ||
| size = size, | ||
| ) |
There was a problem hiding this comment.
_test_wrapper_test is defined without an args attribute, but the macro passes args = .... This will fail at analysis time with an unknown attribute error. Fix by adding an explicit args attribute to _test_wrapper_test and (since a symlinked executable won’t automatically receive those args) generate a small wrapper launcher script (sh/bat as appropriate) that invokes the underlying test_binary with the provided args.
Suggested change
| ) | |
| ) | |
| def _test_wrapper_test( | |
| name, | |
| test_binary, | |
| args, | |
| data = [], | |
| tags = [], | |
| size = None): | |
| """Create a test target that launches `test_binary` with the given args. | |
| This macro generates a small shell wrapper script that invokes the compiled | |
| test binary with the provided `args` and forwards any additional command line | |
| arguments. The wrapper script is then used as the actual test executable. | |
| """ | |
| launcher_name = name + "_launcher" | |
| launcher_script = launcher_name + ".sh" | |
| native.genrule( | |
| name = launcher_name, | |
| srcs = [], | |
| outs = [launcher_script], | |
| tools = [test_binary], | |
| cmd = ( | |
| "echo '#!/usr/bin/env bash' > $@ && " | |
| "echo '\"$(location {test_binary})\" {args} \"$$@\"' >> $@ && " | |
| "chmod +x $@" | |
| ).format( | |
| test_binary = test_binary, | |
| args = " ".join(args), | |
| ), | |
| ) | |
| native.sh_test( | |
| name = name, | |
| srcs = [launcher_script], | |
| data = data + [test_binary], | |
| tags = tags, | |
| size = size, | |
| ) |
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:AllTests | ||
| bazel test //dotnet/test/common |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:ElementFindingTest | ||
| bazel test //dotnet/test/common:ElementFindingTests |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:ElementFindingTest-edge | ||
| bazel test //dotnet/test/common:ElementFindingTests-edge |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| location = allowed.get((platform_name, arch)) | ||
| if location is None: | ||
| raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}") |
There was a problem hiding this comment.
This error message uses sys.platform, but the lookup now normalizes to platform_name for BSD variants. When a combination is unsupported, the message can be misleading (e.g., showing freebsd13/... even if normalization occurred). Prefer reporting the normalized platform_name (and the normalized arch) used for the actual lookup.
Suggested change
| raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}") | |
| raise WebDriverException( | |
| f"Unsupported platform/architecture combination: {platform_name}/{arch}" | |
| ) |
Comment on lines
+88
to
+94
| cmd_parts = [ | ||
| "rm -rf '%s'" % working_dir, | ||
| "mkdir -p '%s'" % working_dir, | ||
| ] + copy_cmds + [ | ||
| "cp '{src}' '{dir}/project.nuspec'".format(src = nuspec.path, dir = working_dir), | ||
| "cp '{src}' '{dir}/project.csproj'".format(src = csproj_file.path, dir = working_dir), | ||
| "cd '%s'" % working_dir, |
There was a problem hiding this comment.
The refactor replaces the zipper-based layout with shell commands (rm, mkdir -p, cp). If this rule is expected to run on Windows hosts (or any environment without a POSIX shell/coreutils), it will break. Consider either (a) restoring a platform-neutral file layout step (zipper or a small cross-platform tool), or (b) using ctx.actions.copy/ctx.actions.write where possible and minimizing reliance on external shell utilities.
| [OneTimeSetUp] | ||
| public async Task RunBeforeAnyTestAsync() | ||
| { | ||
| Log.SetLevel(LogEventLevel.Trace); |
There was a problem hiding this comment.
Setting the global log level to Trace for all webdriver tests can significantly increase output volume and slow down CI/test runs (and may also make failures harder to triage due to noise). Consider gating this behind an environment variable or test run setting, defaulting to a less verbose level.
… the window to the size we want
AutomatedTester
force-pushed
the
cddl2py_bazel
branch
from
753c026 to
9d253d5
Compare
cgoldberg
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
💥 What does this PR do?
Deletes all the hand curated bidi code in the python tree and uses the generated code created in #16914 . Once that PR is approved we can merge this into there or do it into trunk as a 2nd PR. #16914 MUST BE FIRST INTO TRUNK
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes